Checkmarx One – Scan Summary & Details – 597532d8-fb4b-4bb2-b301-4879c1992c74

Fixed Issues (4)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 173
	CSRF	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 217
	CSRF	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 270
	CSRF	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 173

Codecov Report

❌ Patch coverage is 0.63694% with 312 lines in your changes missing coverage. Please review.
✅ Project coverage is 13.82%. Comparing base (d1a5c4d) to head (0abca22).
⚠️ Report is 15 commits behind head on main.

@@             Coverage Diff             @@
##             main    #7079       +/-   ##
===========================================
- Coverage   56.45%   13.82%   -42.64%     
===========================================
  Files        2013     1246      -767     
  Lines       88098    52938    -35160     
  Branches     7860     4089     -3771     
===========================================
- Hits        49739     7318    -42421     
- Misses      36543    45488     +8945     
+ Partials     1816      132     -1684     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.4% Duplication on New Code

See analysis details on SonarQube Cloud

@claude review this pr

Claude finished @prograhamming's task in 4m 44s —— View job

Review of PR #7079 — PM-31920 V2 Report Endpoints

• Gather context and read changed files
• Review API controller changes
• Review core business logic (commands/queries)
• Review storage services (Azure, Local, Noop)
• Review models and entities
• Review test coverage
• Review security considerations
• Post final review

Summary

This PR adds V2 endpoints for organization report CRUD operations behind the WholeReportDataFileStorage feature flag. The V2 flow stores large report data in Azure Blob Storage (or local filesystem for self-hosted) instead of inline in the database, using a SAS-URL-based upload/download pattern. The overall structure follows existing patterns in the codebase (similar to cipher attachment handling). Below are the findings.

Security Issues

1. Path Traversal in LocalOrganizationReportStorageService (Medium)

src/Core/Dirt/Reports/Services/LocalOrganizationReportStorageService.cs:68-72

The RelativePath method uses fileData.Id directly in Path.Combine without sanitizing for path traversal characters. While the file ID is generated server-side via CoreHelpers.SecureRandomString(32) in the create flow, the update flow accepts reportFileId from the query string and validates it matches the stored ID — but this defense is only as strong as the assumption that stored IDs are never tainted.

The test at LocalOrganizationReportStorageServiceTests.cs:103 explicitly demonstrates this vulnerability succeeds with payloads like ../../etc/malicious.

Recommendation: Add path traversal validation in the storage service itself (defense in depth). After constructing the full path, verify it remains under _baseDirPath:

var fullPath = Path.GetFullPath(path);
if (!fullPath.StartsWith(Path.GetFullPath(_baseDirPath)))
    throw new InvalidOperationException("Path traversal detected");

Fix this →

2. [AllowAnonymous] on Azure Validation Endpoint

src/Api/Dirt/Controllers/OrganizationReportsController.cs:439

The AzureValidateFile endpoint is [AllowAnonymous]. This matches the existing pattern in CiphersController.cs:1569, so it's consistent — but it does mean anyone can send crafted EventGrid events to this endpoint. The endpoint does look up the report by ID and deletes orphaned blobs, so a malicious actor could potentially trigger repeated DB lookups. This is an existing design pattern concern rather than something introduced by this PR, so just noting it.

Bugs

3. _getDataV2Query is injected but never used (Bug)

src/Api/Dirt/Controllers/OrganizationReportsController.cs:38,81

IGetOrganizationReportDataV2Query _getDataV2Query is injected into the controller constructor but never called anywhere. The GetOrganizationReportDataAsync GET endpoint at line 263 has no V2 feature-flag branch, meaning there's no way for clients to retrieve a download URL for V2 report data files.

This appears to be a missing feature — the GET endpoint should have a V2 path similar to the create/update endpoints that returns the download URL via _getDataV2Query.

Fix this →

4. Null reference risk in GetOrganizationReportAsync V2 path

src/Api/Dirt/Controllers/OrganizationReportsController.cs:108-113

var report = await _getOrganizationReportQuery.GetOrganizationReportAsync(reportId);
if (report.OrganizationId != organizationId)

While GetOrganizationReportAsync currently throws NotFoundException when null, this code accesses report.OrganizationId without a null check. If the query implementation ever changes, this would be a null reference exception. The V1 path (line 124-128) does check for null. Consider adding a null guard for consistency and safety.

5. Typo in error message

src/Api/Dirt/Controllers/OrganizationReportsController.cs:401

throw new BadRequestException("Invalid contenwt.");

"contenwt" should be "content".

Design Issues

6. Controller is growing very large — consider extracting V2 endpoints

src/Api/Dirt/Controllers/OrganizationReportsController.cs — 20 constructor parameters

The controller now has 20 constructor dependencies and mixes V1 and V2 logic with feature-flag branching throughout. This makes it increasingly hard to test and maintain. Consider extracting the V2 endpoints into a separate controller (e.g., OrganizationReportsV2Controller) gated by [RequireFeature(FeatureFlagKeys.WholeReportDataFileStorage)] at the class level, which would:

• Eliminate per-method feature flag checks
• Allow removing V1 dependencies from V2 controller and vice versa
• Simplify testing
• Make future V1 cleanup easier

Note: The PR originally had OrganizationReportsV2Controller (referenced in the Checkmarx security alerts), so this was apparently an intentional merge back into the single controller. Still worth reconsidering.

7. OrganizationReportFile property appears unused

src/Core/Dirt/Entities/OrganizationReport.cs:30

public ReportFile? OrganizationReportFile { get; set; }

This property is never accessed anywhere in the codebase. File data is stored/retrieved via GetReportFileData()/SetReportFileData() which serialize to/from the ReportData string column. If OrganizationReportFile isn't mapped to a DB column or used by EF, it should be removed to avoid confusion.

8. ReportFile.Validated defaults to true

src/Core/Dirt/Models/Data/ReportFile.cs:29

public bool Validated { get; set; } = true;

The default is true, but CreateOrganizationReportV2Command explicitly sets it to false before validation. A safer default would be false to prevent accidentally treating unvalidated files as validated if the explicit assignment is ever missed.

Minor Issues

9. Inconsistent authorization between V1 and V2 paths

The V2 paths call AuthorizeV2Async which checks both AccessReports and UseRiskInsights, while V1 paths only check AccessReports. This is intentional and correct for feature gating, but the error messages differ: V2 throws NotFoundException for access denied and BadRequestException for missing plan, while V1 throws only NotFoundException. This could be confusing for API consumers — a 403 Forbidden would be more appropriate for the plan check.

10. No controller-level tests for V2 endpoints

The existing OrganizationReportsControllerTests.cs only has minor field-name updates. There are no integration/controller tests for the new V2 create, update, or upload flows. While the command/query tests cover business logic, the controller-level feature-flag branching and authorization checks are untested.

11. AzureOrganizationReportStorageService.InitAsync is not thread-safe

src/Core/Dirt/Reports/Services/AzureOrganizationReportStorageService.cs:113-120

private async Task InitAsync()
{
    if (_containerClient == null)
    {
        _containerClient = _blobServiceClient.GetBlobContainerClient(ContainerName);
        await _containerClient.CreateIfNotExistsAsync(PublicAccessType.None);
    }
}

This is a classic check-then-act race. Multiple concurrent requests could both enter the if block. While CreateIfNotExistsAsync is idempotent, this is still wasteful. Consider using Lazy<Task<BlobContainerClient>> or a SemaphoreSlim for thread-safe initialization. This matches similar patterns already in the codebase for other Azure services.

What Looks Good

• File validation flow with async Azure EventGrid webhook integration follows the established cipher attachment pattern
• Secure random file IDs via CoreHelpers.SecureRandomString(32) with restricted charset
• SAS token lifetime is appropriately short (1 minute)
• Good test coverage on commands/queries with proper null/mismatch/validation cases
• Feature flag gating allows incremental rollout

|